Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cli: move jj co alias to config file, so it can be overridden #3242

Merged
merged 1 commit into from
Mar 7, 2024
Merged

cli: move jj co alias to config file, so it can be overridden #3242

merged 1 commit into from
Mar 7, 2024

Conversation

martinvonz
Copy link
Member

This way users can override jj co to mean jj new if they want to get rid of the warning.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@martinvonz
cli: move jj co alias to config file, so it can be overridden
7055bfd 
This way users can override `jj co` to mean `jj new` if they want to
get rid of the warning.
@martinvonz martinvonz mentioned this pull request Mar 7, 2024
Closed
4 tasks
yuja
yuja approved these changes Mar 7, 2024
View reviewed changes
Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@martinvonz
Copy link
Member Author

@thoughtpolice, @PhilipMetzger: are you okay with this?

thoughtpolice
thoughtpolice approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@thoughtpolice thoughtpolice left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, LGTM. I should have done this in the original PR, honestly, since I wanted to free up these names as early as possible so people could reuse co. Thanks!

PhilipMetzger
PhilipMetzger reviewed Mar 7, 2024
View reviewed changes
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG. I think for the future, we should just provide the aliases as built-in config, instead of defining them via clap. Then everyone is free to define them as is.

@martinvonz martinvonz merged commit 663e352 into main Mar 7, 2024
16 checks passed
@martinvonz martinvonz deleted the push-wwqsqtkqqmtw branch March 7, 2024 17:46
@martinvonz
Copy link
Member Author

I think for the future, we should just provide the aliases as built-in config, instead of defining them via clap.

I hesitate to do that for two reasons:

  • Aliases configured in clap could show up in the help text, though it seems that clap doesn't currently do that (it does for flag aliases)
  • By having a builtin alias like jj undo, we can refer to that in hints and such. Maybe it's still fine to do that and just tell people who define aliases.undo = ["new"] to not shoot themselves in the foot

@PhilipMetzger
Copy link
Contributor

PhilipMetzger commented Mar 7, 2024 •
edited
Loading

I don't mind having a set of core aliases built-in but convenience aliases should be defined in the config.

@PhilipMetzger PhilipMetzger mentioned this pull request Apr 26, 2024
Open
4 tasks
@PhilipMetzger PhilipMetzger mentioned this pull request Jun 17, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilipMetzger PhilipMetzger PhilipMetzger approved these changes

@yuja yuja yuja approved these changes

@thoughtpolice thoughtpolice thoughtpolice approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martinvonz @PhilipMetzger @thoughtpolice @yuja